Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adjust ok_or and map_or to _else for lazy computation #14764

Merged
merged 1 commit into from
Oct 11, 2024
Merged

Conversation

gelash
Copy link
Contributor

@gelash gelash commented Sep 26, 2024

Audit and adjust the uses, e.g. where there is error preparation or string formatting on the unhappy path, to make sure it doesn't execute eagerly on the happy path.

How Has This Been Tested?

passes existing

Type of Change

  • New feature
  • Bug fix
  • Breaking change
  • Performance improvement
  • Refactoring
  • Dependency update
  • Documentation update
  • Tests

Which Components or Systems Does This Change Impact?

  • Validator Node
  • Full Node (API, Indexer, etc.)
  • Move/Aptos Virtual Machine
  • Aptos Framework
  • Aptos CLI/SDK
  • Developer Infrastructure
  • Move Compiler
  • Other (specify)

Copy link

trunk-io bot commented Sep 26, 2024

⏱️ 2h total CI duration on this PR
Slowest 15 Jobs Cumulative Duration Recent Runs
execution-performance / single-node-performance 44m 🟩🟩
rust-move-unit-coverage 17m 🟩
forge-e2e-test / forge 15m 🟩
rust-move-tests 10m 🟩
test-target-determinator 8m 🟩🟩
execution-performance / test-target-determinator 8m 🟩🟩
rust-move-tests 8m
rust-cargo-deny 3m 🟩🟩
check-dynamic-deps 2m 🟩🟩🟩
semgrep/ci 1m 🟩🟩🟩
general-lints 56s 🟩🟩
file_change_determinator 32s 🟩🟩
file_change_determinator 26s 🟩🟩
Backport PR 10s 🟥🟥
permission-check 9s 🟩🟩🟩

🚨 1 job on the last run was significantly faster/slower than expected

Job Duration vs 7d avg Delta
general-lints 26s 2m -74%

settingsfeedbackdocs ⋅ learn more about trunk.io

@gelash gelash added CICD:run-execution-performance-test Run execution performance test CICD:run-forge-e2e-perf Run the e2e perf forge only labels Sep 26, 2024
Copy link

codecov bot commented Sep 26, 2024

Codecov Report

Attention: Patch coverage is 30.30303% with 23 lines in your changes missing coverage. Please review.

Project coverage is 60.0%. Comparing base (918b643) to head (9598961).
Report is 12 commits behind head on main.

Files with missing lines Patch % Lines
...ptos-vm/src/verifier/transaction_arg_validation.rs 0.0% 8 Missing ⚠️
...s-move/block-executor/src/txn_last_input_output.rs 0.0% 7 Missing ⚠️
aptos-move/block-executor/src/executor.rs 0.0% 4 Missing ⚠️
crates/aptos-dkg/src/weighted_vuf/pinkas/mod.rs 0.0% 2 Missing ⚠️
types/src/block_info.rs 0.0% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff            @@
##             main   #14764     +/-   ##
=========================================
- Coverage    60.1%    60.0%   -0.1%     
=========================================
  Files         856      856             
  Lines      210845   210892     +47     
=========================================
- Hits       126742   126737      -5     
- Misses      84103    84155     +52     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

This comment has been minimized.

This comment has been minimized.

@gelash gelash changed the title ok_or and map_or audit Adjust ok_or and map_or to _else for lazy computation Sep 26, 2024
@gelash gelash marked this pull request as ready for review September 26, 2024 14:00
@gelash gelash requested a review from igor-aptos September 26, 2024 14:01
Copy link
Contributor

@lightmark lightmark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@banool
Copy link
Contributor

banool commented Sep 26, 2024

A pass for .context() could be good too, I see some cases where we're using .context(format!()) for example, we should perhaps use .with_context() instead.

@gelash
Copy link
Contributor Author

gelash commented Sep 26, 2024

there is also straight up or( that I didn't look for yet

let mut prev_modified_keys = last_input_output
.modified_keys(idx_to_execute)
.map_or(HashMap::new(), |keys| keys.collect());
let mut prev_modified_keys = match last_input_output.modified_keys(idx_to_execute) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not just map_or_else?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issues with type deduction somehow

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if there's error you just need to annotate the types like keys.collect::<HashMap<,>>() but I also don't see errors when trying it

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried annotating and kept seeing weird errors, will try again

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

worked out, must have been on some weird local state

types/src/block_info.rs Outdated Show resolved Hide resolved
let output_prefix = options.move_sources.first().map_or("bytecode", |s| {
Path::new(s).file_name().unwrap().to_str().unwrap()
});
let output_prefix = options.move_sources.first().map_or_else(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

const literals are cheaper than functions?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

true, missed this one, usually the linter would complain in this case. will revert.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you have this in multiple places as far as I can see

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will do a pass. For sure left a bunch unchanged and got linter errors and reverted on some. string literal def can stay in or(

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I got all of them

let mut prev_modified_keys = last_input_output
.modified_keys(idx_to_execute)
.map_or(HashMap::new(), |keys| keys.collect());
let mut prev_modified_keys = match last_input_output.modified_keys(idx_to_execute) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if there's error you just need to annotate the types like keys.collect::<HashMap<,>>() but I also don't see errors when trying it

aptos-move/block-executor/src/txn_last_input_output.rs Outdated Show resolved Hide resolved
crates/aptos/src/move_tool/mod.rs Outdated Show resolved Hide resolved
mempool/src/core_mempool/mempool.rs Outdated Show resolved Hide resolved
@gelash gelash force-pushed the gelash/orfix branch 2 times, most recently from 287eb78 to 54c63b7 Compare October 6, 2024 13:49

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

@gelash gelash enabled auto-merge (squash) October 11, 2024 17:03

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Copy link
Contributor

✅ Forge suite realistic_env_max_load success on 9598961adf05a96ac80daddf8e3b9afb2e147185

two traffics test: inner traffic : committed: 13430.37 txn/s, latency: 2961.40 ms, (p50: 2700 ms, p70: 3000, p90: 3300 ms, p99: 5100 ms), latency samples: 5106560
two traffics test : committed: 99.98 txn/s, latency: 2944.81 ms, (p50: 2500 ms, p70: 2600, p90: 3800 ms, p99: 11300 ms), latency samples: 1680
Latency breakdown for phase 0: ["QsBatchToPos: max: 0.235, avg: 0.219", "QsPosToProposal: max: 0.281, avg: 0.235", "ConsensusProposalToOrdered: max: 0.328, avg: 0.300", "ConsensusOrderedToCommit: max: 0.489, avg: 0.468", "ConsensusProposalToCommit: max: 0.784, avg: 0.768"]
Max non-epoch-change gap was: 0 rounds at version 0 (avg 0.00) [limit 4], 1.24s no progress at version 49442 (avg 0.21s) [limit 15].
Max epoch-change gap was: 0 rounds at version 0 (avg 0.00) [limit 4], 8.17s no progress at version 2786911 (avg 8.17s) [limit 15].
Test Ok

Copy link
Contributor

✅ Forge suite compat success on beff51858b445401e49d5be352feadcf05652cc0 ==> 9598961adf05a96ac80daddf8e3b9afb2e147185

Compatibility test results for beff51858b445401e49d5be352feadcf05652cc0 ==> 9598961adf05a96ac80daddf8e3b9afb2e147185 (PR)
1. Check liveness of validators at old version: beff51858b445401e49d5be352feadcf05652cc0
compatibility::simple-validator-upgrade::liveness-check : committed: 12953.78 txn/s, latency: 2243.79 ms, (p50: 1800 ms, p70: 2000, p90: 2400 ms, p99: 20800 ms), latency samples: 508540
2. Upgrading first Validator to new version: 9598961adf05a96ac80daddf8e3b9afb2e147185
compatibility::simple-validator-upgrade::single-validator-upgrading : committed: 7008.70 txn/s, latency: 4150.99 ms, (p50: 4200 ms, p70: 4600, p90: 4800 ms, p99: 4900 ms), latency samples: 131980
compatibility::simple-validator-upgrade::single-validator-upgrade : committed: 7094.39 txn/s, latency: 4576.58 ms, (p50: 5000 ms, p70: 5100, p90: 5300 ms, p99: 5400 ms), latency samples: 235240
3. Upgrading rest of first batch to new version: 9598961adf05a96ac80daddf8e3b9afb2e147185
compatibility::simple-validator-upgrade::half-validator-upgrading : committed: 7277.33 txn/s, latency: 3789.41 ms, (p50: 4300 ms, p70: 4800, p90: 5100 ms, p99: 5300 ms), latency samples: 129700
compatibility::simple-validator-upgrade::half-validator-upgrade : committed: 7402.65 txn/s, latency: 4298.30 ms, (p50: 4400 ms, p70: 4600, p90: 6200 ms, p99: 6700 ms), latency samples: 247440
4. upgrading second batch to new version: 9598961adf05a96ac80daddf8e3b9afb2e147185
compatibility::simple-validator-upgrade::rest-validator-upgrading : committed: 11164.01 txn/s, latency: 2415.06 ms, (p50: 2700 ms, p70: 2800, p90: 2900 ms, p99: 3100 ms), latency samples: 199240
compatibility::simple-validator-upgrade::rest-validator-upgrade : committed: 11022.32 txn/s, latency: 2850.35 ms, (p50: 2700 ms, p70: 2800, p90: 3000 ms, p99: 5500 ms), latency samples: 358780
5. check swarm health
Compatibility test for beff51858b445401e49d5be352feadcf05652cc0 ==> 9598961adf05a96ac80daddf8e3b9afb2e147185 passed
Test Ok

Copy link
Contributor

✅ Forge suite framework_upgrade success on beff51858b445401e49d5be352feadcf05652cc0 ==> 9598961adf05a96ac80daddf8e3b9afb2e147185

Compatibility test results for beff51858b445401e49d5be352feadcf05652cc0 ==> 9598961adf05a96ac80daddf8e3b9afb2e147185 (PR)
Upgrade the nodes to version: 9598961adf05a96ac80daddf8e3b9afb2e147185
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1175.93 txn/s, submitted: 1178.13 txn/s, failed submission: 2.20 txn/s, expired: 2.20 txn/s, latency: 2507.93 ms, (p50: 2400 ms, p70: 2700, p90: 3300 ms, p99: 5300 ms), latency samples: 106980
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1193.76 txn/s, submitted: 1195.77 txn/s, failed submission: 2.01 txn/s, expired: 2.01 txn/s, latency: 2498.92 ms, (p50: 2400 ms, p70: 2700, p90: 3600 ms, p99: 4800 ms), latency samples: 106800
5. check swarm health
Compatibility test for beff51858b445401e49d5be352feadcf05652cc0 ==> 9598961adf05a96ac80daddf8e3b9afb2e147185 passed
Upgrade the remaining nodes to version: 9598961adf05a96ac80daddf8e3b9afb2e147185
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1137.23 txn/s, submitted: 1139.44 txn/s, failed submission: 2.21 txn/s, expired: 2.21 txn/s, latency: 2614.35 ms, (p50: 2400 ms, p70: 2700, p90: 4500 ms, p99: 6900 ms), latency samples: 102840
Test Ok

@gelash gelash merged commit a822e65 into main Oct 11, 2024
95 of 96 checks passed
@gelash gelash deleted the gelash/orfix branch October 11, 2024 17:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CICD:run-execution-performance-test Run execution performance test CICD:run-forge-e2e-perf Run the e2e perf forge only
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants